Given that the existing methods are called -mapView:didSelectAnnotation: and -mapView:didDeselectAnnotation:, without referring to point annotations specifically, it’s interesting that this PR adds separate shape-specific methods.

On the one hand, I understand that suddenly passing shape annotations into a method that has long received only point annotations could be a surprise to some developers. On the other hand, we’ve never documented that prior limitation – it’s essentially a bug. There is precedent for opening up delegate methods to new data, namely when we started passing MGLUserLocation into -mapView:viewForAnnotation: in #5882.

In #9755 (comment) I mention an alternative

The reason behind naming this as didSelectShapeAnnotation is based on been clear to developers that this is not going to be called by selecting any *MGLPointAnnotation thus there is no callout view.

The @"" string literal NSString is already immutable. So this should be a constant pointer. As it stands, you can repoint MGLLayerIDShapeAnnotation and change its value. Also, this is only used in the implementation so it can be designated static. (i.e. static NSString *const MGLFoo = @"foo")

I think it'd help readability later on to follow the convention (see _annotationTagsByAnnotation just above) and call this _shapeAnnotationLayerIDsByAnnotation

Tapping a map with no annotations now crashes in -[MGLMapView shapeAnnotationForGestureRecognizer:] because the hitAnnotationTag is 0 (instead of MGLAnnotationTagNotFound) and that gets a nil result from [self annotationWithTag:hitAnnotationTag] which triggers an assert. I think the root cause is that -[MGLMapView shapeAnnotationTagsInRect:] has no _shapeAnnotationLayerIDs values to put in options so the _mbglMap->queryShapeAnnotations() actually returns some values instead of an empty list.

Fixed. Thank you for catching this bug.

These new APIs don't provide a shape "annotation". They do provide an MGLShape. Since they are intended for MGLPolyline(gon)(Feature) instances, should we remove the word "annotation" from the API and use a more specific type?

MGLShape conforms to MGLAnnotation.

These commented lines were pulled into methods. Don't forget to delete them here.

The older -[MGLMapView annotationTagAtPoint:persistingResults:] method has logic to cycle through points that re overlapping / nearby each other. Do we need to worry about that for shapes? What is the expected behavior if a user touches in an area where several polygons overlap or several polylines intersect? cc @1ec5

It’s currently possible to disable touch interaction for a given point annotation using either MGLAnnotationImage.enabled. That’s a weird API, but do we need something like it for shape annotations?

The intent of #2082 was that we’d display a callout over the selected shape. Per #2082 (comment), we’d need to constrain the coordinate to the current viewport before calculating the centroid. So coordinate wouldn’t give us quite the right behavior, but we could fashion a method that takes an MGLCoordinateBounds and clips the polygon to polyline to that bounds before passing the geometry off to Polylabel.

The intent of #2082 was that we’d display a callout over the selected shape.

Our Android SDK gained essentially the exact same functionality that is proposed here in #9443. I don't think there is any mechanism in that SDK to assist with callout placement, yet. I think it would be ok to use the same approach and keep #2082 open and handle callouts separately.

I guess that would be an argument for adding a separate set of delegate methods, because it would be weird for -mapView:didSelectAnnotation: to work with -mapView:annotationCanShowCallout: in some cases but not others.

It doesn’t feel great to potentially have a point annotation and shape annotation both be selected at the same time. It’ll lock us into a design that’s reminiscent of MapKit but quite different, even after we implement the callout views.

My principal deterrent to avoid using [MGLMapView annotationTagAtPoint:persistingResults:] was the fact that *MGLShape "annotations" does not have a callout view. So selecting an annotation that handles a callout only in specific cases would be confusing for developers and interpreted as a bug.

But selecting a MGLShape annotation deselects MGLPointAnnotation and viceversa.

I think the broader question is whether shapes should have callouts. The original request in #2082 and much of the subsequent comments were about callouts, so I guess I’d consider that PR unfinished without them. If we land this feature without callouts and with an API that’s separate from normal annotation selection, does that make it more difficult for us to implement shape callouts later? Or are we counting on being able to clean up this API in v4.0.0?

Right, the original request in #2082 was callouts. I should have referred to #3720 which is what the Android implementation #9443 fixes.

Maybe we could wait and refactor our Annotation API, then support callouts for shape annotations, this is how it looks right know so far.

How much of a lift would it be to support callouts for shape annotations? At a glance, it looks like most of the pieces are already in place (especially polylabel).